-
-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip Alter ADD CONSTRAINT
when changing the table schema
#39
Conversation
ADD CONSTRAINT
when changing the table schema
sqlite3.go
Outdated
_, ok := def.(rel.Key) | ||
// https://www.sqlite.org/omitted.html | ||
// > Only the RENAME TABLE, ADD COLUMN, RENAME COLUMN, and DROP COLUMN variants of the ALTER TABLE command are supported. | ||
if ok && table.Op == rel.SchemaAlter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this skip RENAME TABLE, ADD COLUMN, RENAME COLUMN, and DROP COLUMN
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. I added tests to verify that they are not skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I missed _, ok := def.(rel.Key)
can you move above statement inside if table.Op == rel.SchemaAlter
? so there's no need to try casting when Op is not SchemaAlter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you
ADD CONSTRAINT
when changing the table schemaADD CONSTRAINT
when changing the table schema
related: go-rel/sql#39